-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DE-101: Re-platform application to Python #11
Conversation
Should I update the |
Also @aaronfriedman6 I can't update the settings for this repo so that we can perform Python unit tests in the checks rip |
You can remove everything related to Travis -- we don't use it anymore. Instead, we use GitHub actions to handle deployments.
What settings need to be changed? |
Nvm I think I figured it out! In the past I've had to hook up running tests automatically through the Github settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Some small things but nothing major. I haven't reviewed the tests yet but figured I'd wait in case any of my comments required changing the tests too
config/sam.qa.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file used by the python lambda package you mentioned? Also remove SCHEMA_NAME
var below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be used for sam local
or the python lambda local usage! This is mainly a holdover from the NodeJS implementation, since we have plenty of sample events (in the sample
folder) to use and verify when running local changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does sam local
or the python lambda package actually run the QA Lambda function or does it just mimic it? Like when you run it could I go on AWS and see the logs from the run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sam local
mimics the Lambda function locally and stores the logs locally. So this will not be visible on AWS. When you run the command, the output shows up on the terminal.
record_processor.py
Outdated
# After decoding, convert to base64. More often than not, | ||
# the original data is either encoded or not in base64 | ||
to_bytes = data.encode("utf-8") | ||
return (base64.b64encode(to_bytes)).decode("utf-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't totally get this -- could explain a little more what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little convoluted but we need to encode the decoded data to base64, and base64.b64encode()
requires a bytes-like input. In addition, the .decode("utf-8")
removes the byte wrapper around the decoded output. I.e. transforms b'Y2N8TGlicmFyeSBcfCBDICEsPXxXZWR8MDE6MDB8MjM6MDB8MjAyMy0wMy0wMwo='
to Y2N8TGlicmFyeSBcfCBDICEsPXxXZWR8MDE6MDB8MjM6MDB8MjAyMy0wMy0wMwo=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So data
is a regular string, then you convert it to UTF-8 bytes, then you convert those UTF-8 bytes to hex bytes, then you convert those hex bytes to a hex string? Could you add a comment saying that data
is a regular string and we want to output that same string in hex, which requires using bytes as an intermediate type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. I'll add the comment in rn!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great -- I'm ready to see it in action!
config/sam.qa.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does sam local
or the python lambda package actually run the QA Lambda function or does it just mimic it? Like when you run it could I go on AWS and see the logs from the run?
Assigning it to you one last time since I replied to some of your comments! Also, should I merge to main after this? |
Looks good! Yep merge to main and we'll talk offline about how to deploy this |
Changes made
Related story
DE-101
Initially assigning just so you can take a look at the code! Won't merge in until python utils is merged